[pipelines] add PNPM_CONFIG_REGISTRY endpoint - #11700
Open
iscai-msft wants to merge 2 commits into
Open
Conversation
microsoft-github-policy-service
Bot
added
emitter:client:csharp
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates TypeSpec’s publishing/regeneration pipelines and Docker build to support pnpm v11+ configuration via pnpm_config_* / PNPM_CONFIG_* variables, including allowing a registry endpoint override for compliance scenarios.
Changes:
- Propagate
.npmrcvia bothNPM_CONFIG_USERCONFIGandPNPM_CONFIG_USERCONFIGfor tsp-client-driven installs. - Write both
npm_config_registryandpnpm_config_registryinto a temporary.envfor regeneration flows. - Add an overridable
NPM_REGISTRYpipeline variable and pass it into the Docker build as--build-arg NPM_REGISTRY=..., plus set pnpm/npm registry env vars in the Dockerfile.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/http-client-csharp/eng/scripts/Submit-AzureSdkForNetPr.ps1 | Sets NPM_CONFIG_USERCONFIG and PNPM_CONFIG_USERCONFIG so tsp-client can resolve packages using the repo .npmrc. |
| packages/http-client-csharp/eng/scripts/RegenPreview.ps1 | Writes both npm and pnpm registry env vars to a temporary .env for tsp-client execution. |
| packages/http-client-csharp/eng/pipeline/publish.yml | Adds pnpm-related pipeline variables (store/cache/force) alongside npm variables. |
| eng/tsp-core/pipelines/publish.yml | Introduces an npmRegistry variable (overridable via NPM_REGISTRY) and passes it as a Docker build-arg. |
| docker/Dockerfile | Accepts NPM_REGISTRY build-arg and sets npm/pnpm registry env vars for dependency installs in the build stages. |
Suppressed comments (1)
docker/Dockerfile:37
- The final stage also defaults
ARG NPM_REGISTRY=to empty and then setsNPM_CONFIG_REGISTRYto that value. For docker builds that don’t pass the build-arg, this can force npm’s registry to an empty value duringnpm install -g /tmp/compiler.tgz, which can break dependency resolution.
# Inherit registry setting for final stage
ARG NPM_REGISTRY=
ENV NPM_CONFIG_REGISTRY=${NPM_REGISTRY}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+7
to
+12
| # Optional: Override npm registry for CFS compliance. | ||
| # pnpm 11+ requires PNPM_CONFIG_REGISTRY (ignores NPM_CONFIG_REGISTRY). | ||
| ARG NPM_REGISTRY= | ||
| ENV PNPM_CONFIG_REGISTRY=${NPM_REGISTRY} | ||
| ENV NPM_CONFIG_REGISTRY=${NPM_REGISTRY} | ||
|
|
- Use underscore in pnpm_config_store_dir (not hyphen) - Default NPM_REGISTRY to https://registry.npmjs.org/ instead of empty
| $originalSdkEnv = if (Test-Path $sdkEnvFile) { Get-Content $sdkEnvFile -Raw } else { $null } | ||
| Set-Content $sdkEnvFile "npm_config_registry=$artifactFeedRegistry`n" -Encoding utf8 -NoNewline | ||
| # Set both npm and pnpm registry env vars (pnpm 11+ ignores npm_config_* prefix) | ||
| Set-Content $sdkEnvFile "npm_config_registry=$artifactFeedRegistry`npnpm_config_registry=$artifactFeedRegistry`n" -Encoding utf8 -NoNewline |
Contributor
There was a problem hiding this comment.
we don't use pnpm when running this script. It's primarily used locally. Do we need this change here? If so, do we need to add some delimiter between the 2 registry values ?
commit: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docker/Dockerfile:36
- Final Docker image stage only sets NPM_CONFIG_REGISTRY, but the earlier comment states pnpm 11+ requires PNPM_CONFIG_REGISTRY. If any runtime path uses pnpm (e.g.,
tsp installor future additions), the overridden registry won't apply in the final image. Set PNPM_CONFIG_REGISTRY in the final stage as well to truly inherit the registry setting.
# Inherit registry setting for final stage
ARG NPM_REGISTRY=https://registry.npmjs.org/
ENV NPM_CONFIG_REGISTRY=${NPM_REGISTRY}
|
You can try these changes here
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.